Tdk invensense icm42x70 support 3axis #83498
Conversation
zephyrbot
requested review from
asemjonovs,
avisconti,
jeppenodgaard,
kartben,
MaureenHelm,
nashif,
teburd,
tristan-google,
ubieda and
yperess
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
zephyrbot
added
manifest
manifest-hal_tdk
DNM
afontaine-invn
force-pushed
the
TDK_Invensense_icm42x70_support_3axis
branch
4 times, most recently
from
3843b70 to
a9178fe
Compare
There was a problem hiding this comment.
Great work!
Haven't had a deep look from an actual driver perspective but my initial comment would be that you need to please describe in the Zephyr 4.1 migration guide the changes users of the existing driver/compatible will have to make to their codebase since things will "break" for them due to some of the refactoring -- it looks like it might be pretty minimal (so kudos on that!) but you should e.g. describe things like the fact that the public header for sensor custom attribute has been renamed include/zephyr/drivers/sensor/icm42670.h → include/zephyr/drivers/sensor/icm42x70.h ; things like that.
afontaine-invn
force-pushed
the
TDK_Invensense_icm42x70_support_3axis
branch
from
a9178fe to
36e1d18
Compare
afontaine-invn
force-pushed
the
TDK_Invensense_icm42x70_support_3axis
branch
from
36e1d18 to
dc6fc86
Compare
kartben
dismissed
their stale review
my comments have been addressed - over to sensor maintainers and collaborators for more in-depth review :)
afontaine-invn
force-pushed
the
TDK_Invensense_icm42x70_support_3axis
branch
3 times, most recently
from
59d055c to
007341c
Compare
afontaine-invn
force-pushed
the
TDK_Invensense_icm42x70_support_3axis
branch
3 times, most recently
from
f6d46c4 to
76ec1d7
Compare
|
Hi @MaureenHelm , |
| @@ -377,7 +381,8 @@ static int icm42670_accel_config(struct icm42670_data *drv_data, enum sensor_att | |||
| return 0; | |||
| } | |||
|
|
|||
| static int icm42670_set_gyro_odr(struct icm42670_data *drv_data, const struct sensor_value *val) | |||
| #if CONFIG_USE_EMD_ICM42670 | |||
There was a problem hiding this comment.
Suggestion, to make the code more readable add these functions to icm42670.h and include it from icm42x70.h, then move the definitions to an icm42670.c file and guard the inclusion of it in the cmake file. I generally find that it's easier to read the code that way.
There was a problem hiding this comment.
Thanks for this suggestion. Indeed, it lightweight the icm42x70.c file, it's easy to read. And icm42670 dedicated code is identified. I did the change.
| (chan == SENSOR_CHAN_ACCEL_X) || (chan == SENSOR_CHAN_ACCEL_Y) || | ||
| (chan == SENSOR_CHAN_ACCEL_Z) || (chan == SENSOR_CHAN_GYRO_XYZ) || | ||
| (chan == SENSOR_CHAN_GYRO_X) || (chan == SENSOR_CHAN_GYRO_Y) || | ||
| (chan == SENSOR_CHAN_GYRO_Z) || (chan == SENSOR_CHAN_DIE_TEMP)) { | ||
| #ifdef CONFIG_ICM42670_TRIGGER | ||
| status = icm42670_fetch_from_fifo(dev); | ||
| (chan == SENSOR_CHAN_ACCEL_Z) |
There was a problem hiding this comment.
nit: prefer sensor.h's SENSOR_CHANNEL_IS_ACCEL(chan) and SENSOR_CHANNEL_IS_GYRO(chan) for readability.
There was a problem hiding this comment.
Thanks for this suggestion also, it's more elegant. I did the change
afontaine-invn
force-pushed
the
TDK_Invensense_icm42x70_support_3axis
branch
3 times, most recently
from
a0367cd to
6651aab
Compare
The icm42370-p is a 3-axis MEMS accelermeter. https://invensense.tdk.com/products/motion-tracking/3-axis/icm-42370-p/ Signed-off-by: Aurelie Fontaine <[email protected]>
To support icm42370-p sensor, we renamed icm42670 to icm42x70 as the driver is similar to ICM-42670-P/S in hal_tdk module. Keep icm42670 source and header for dedicated code. Signed-off-by: Aurelie Fontaine <[email protected]>
Add icm42x70 device tree file, it includes the common properties for icm42370-p, icm42670-p, and icm42670-s. Add icm42370-p device tree files for SPI and I2C interface. Signed-off-by: Aurelie Fontaine <[email protected]>
Rename ICM42670 define to ICM42X70 to be aligned with updated driver to support icm42370-p Signed-off-by: Aurelie Fontaine <[email protected]>
Rename ICM42670 define to ICM42X70 to be aligned with updated driver to support icm42370-p Signed-off-by: Aurelie Fontaine <[email protected]>
Rename ICM42670 define to ICM42X70 to be aligned with updated driver to support icm42370-p Signed-off-by: Aurelie Fontaine <[email protected]>
afontaine-invn
force-pushed
the
TDK_Invensense_icm42x70_support_3axis
branch
from
6651aab to
4fcdb7e
Compare
| @@ -490,6 +397,7 @@ static int icm42670_turn_on_sensor(const struct device *dev) | |||
| } | |||
| LOG_DBG("Set accel full scale to: %d G", data->accel_fs); | |||
|
|
|||
| #if CONFIG_USE_EMD_ICM42670 | |||
There was a problem hiding this comment.
This won't work if there is both an icm42670 and an icm42370 in the system. You need to check if this device is an icm42670, not if any device is an icm42670.
The icm42370-p is a 3-axis MEMS accelerometer.
https://invensense.tdk.com/products/motion-tracking/3-axis/icm-42370-p/
To support icm42370-p sensor, we renamed icm42670 to icm42x70 as the driver is similar to ICM-42670-P/S in hal_tdk module.
Add icm42x70 device tree file, it includes the common properties for icm42370-p, icm42670-p, and icm42670-s. Add icm42370-p device tree files for SPI and I2C interface.
Signed-off-by: Aurelie Fontaine [email protected]